-
Notifications
You must be signed in to change notification settings - Fork 14.5k
[mlir][tosa] Interpret boolean values correctly in cast folder #147078
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-mlir-tosa @llvm/pr-subscribers-mlir Author: Luke Hutton (lhutton1) ChangesPreviously the cast folder would sign extend boolean values, leading "true" to be casted to a value of -1 instead of 1. This change ensures i1 values are zero extended, since i1 is used as a boolean value in TOSA. According to the TOSA spec, the result of a boolean cast with value "true" to another integer type should give a result of 1. Fixes #57951 Full diff: https://github.com/llvm/llvm-project/pull/147078.diff 2 Files Affected:
diff --git a/mlir/lib/Dialect/Tosa/IR/TosaCanonicalizations.cpp b/mlir/lib/Dialect/Tosa/IR/TosaCanonicalizations.cpp
index 1d21096e8920b..e2a2f2935b6b4 100644
--- a/mlir/lib/Dialect/Tosa/IR/TosaCanonicalizations.cpp
+++ b/mlir/lib/Dialect/Tosa/IR/TosaCanonicalizations.cpp
@@ -1301,7 +1301,8 @@ OpFoldResult CastOp::fold(FoldAdaptor adaptor) {
}
if (llvm::isa<IntegerType>(inETy) && llvm::isa<IntegerType>(outETy)) {
- auto unsignIn = llvm::cast<IntegerType>(inETy).isUnsignedInteger();
+ const auto inIntType = llvm::cast<IntegerType>(inETy);
+ auto unsignIn = inIntType.isUnsignedInteger();
bool trunc =
inETy.getIntOrFloatBitWidth() > outETy.getIntOrFloatBitWidth();
auto intVal = operand.getSplatValue<APInt>();
@@ -1309,7 +1310,8 @@ OpFoldResult CastOp::fold(FoldAdaptor adaptor) {
if (trunc) {
intVal = intVal.trunc(bitwidth);
- } else if (unsignIn) {
+ // i1 types are boolean in TOSA
+ } else if (unsignIn || inIntType.isInteger(1)) {
intVal = intVal.zext(bitwidth);
} else {
intVal = intVal.sext(bitwidth);
diff --git a/mlir/test/Dialect/Tosa/canonicalize.mlir b/mlir/test/Dialect/Tosa/canonicalize.mlir
index 27280807b0282..11c8d54fda055 100644
--- a/mlir/test/Dialect/Tosa/canonicalize.mlir
+++ b/mlir/test/Dialect/Tosa/canonicalize.mlir
@@ -1338,3 +1338,14 @@ func.func @no_fold_mul_result_exceeds_i32() -> tensor<i32> {
%3 = tosa.mul %0, %1, %2 : (tensor<i32>, tensor<i32>, tensor<1xi8>) -> tensor<i32>
return %3 : tensor<i32>
}
+
+// -----
+
+// CHECK-LABEL: @test_fold_i1_to_i32_cast
+// CHECK: %[[OUT:.*]] = "tosa.const"() <{values = dense<1> : tensor<i32>}> : () -> tensor<i32>
+// CHECK: return %[[OUT]] : tensor<i32>
+func.func @test_fold_i1_to_i32_cast() -> tensor<i32> {
+ %0 = "tosa.const"() <{values = dense<1> : tensor<i1>}> : () -> tensor<i1>
+ %1 = "tosa.cast"(%0) : (tensor<i1>) -> tensor<i32>
+ return %1 : tensor<i32>
+}
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
55a1117
to
9c92a87
Compare
Previously the cast folder would sign extend boolean values, leading "true" to be casted to a value of -1 instead of 1. This change ensures i1 values are zero extended, since i1 is used as a boolean value in TOSA. According to the TOSA spec, the result of a boolean cast with value "true" to another integer type should result in "1". Fixes llvm#57951 Change-Id: I21486cae0b8ad1cf7901e7b3eb92c1ad56c3797c
9c92a87
to
3eebd3f
Compare
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/24/builds/10580 Here is the relevant piece of the build log for the reference
|
thanks for fixing this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Previously the cast folder would sign extend boolean values, leading "true" to be casted to a value of -1 instead of 1. This change ensures i1 values are zero extended, since i1 is used as a boolean value in TOSA. According to the TOSA spec, the result of a boolean cast with value "true" to another integer type should give a result of 1.
Fixes #57951